Feat: Lineage telemetry plugin — two facts-only spans per exchange - #761

conn is created on line 158 but never closed when otlptracegrpc.New returns an error here. Suggest adding _ = conn.Close() (or tracking with a cleanup flag) before the early return to avoid leaking the gRPC connection on any Init failure path after the dial succeeds.

Fixed in 4f4e31c6. The otlptracegrpc.New error path now closes the dialed conn before returning (plugin.go:225, _ = conn.Close()), and p.conn is only stored past that error path, so a failure there no longer leaks the gRPC client. Shutdown closes it on the success path. Thanks.

strings.Contains(kind, "status") is broader than needed and could suppress output for a user-defined artifact kind that incidentally contains the word status (e.g. "final-status-report"). The A2A protocol event kinds are enumerated; prefer exact equality checks: kind == "status-update" || kind == "task-status-update" || kind == "artifact-update" || kind == "working" || kind == "canceled". The redundant strings.Contains(kind, "Status") (capital-S) also suggests this predicate grew ad hoc.

Fixed in 00713329. isA2AProtocolEvent now matches the enumerated kinds exactly via a switch/case (status-update, task-status-update, artifact-update, working, canceled) instead of strings.Contains, and the redundant capital-Status check is gone — so a user artifact whose kind merely contains the word (your final-status-report example) is no longer suppressed. The comment there now cites that exact case. Thanks for the catch.

must-fix (blocking in substance) — this line is a silent no-op on main today, and the failure is indistinguishable from healthy operation.

pctx.Headers.Set("tracestate", ...) only reaches the wire on listeners that propagate the full header set. On current main:

This PR's history is two commits and contains none of #760's, so merged on its own the outbound peer stamping never leaves the sidecar — and that is the mechanism the entire two-span pairing model rests on.

What makes it worth blocking on rather than noting: the degradation is invisible. selectParent falls back to the wire parent and records lineage.parent.source=wire, which the package doc describes as a legitimate state ("Un-stamped traffic falls to the wire parent... the interaction still derives in full, but as a trace entry rather than a child"). So a deployment would look healthy while producing a systematically flattened graph, with nothing in the logs to say why.

No code change needed — declare the dependency and sequence #760 before #761. Worth stating in the PR body too, since #760's own description frames the header fix as "a correctness fix to your own plugins, independent of anything we run", which is true on its own terms but reads as though nothing downstream depends on it.

This is resolved on the branch as it stands — the sequencing dependency you identified no longer exists, because #760 is already merged in here (merge 2349bfeb, plus 4440ef96 "Propagate every plugin header mutation in extproc and forwardproxy"). Your table was accurate against the main of the time, but this branch now carries the full-header-set propagation on all three listeners:

• reverseproxy — full-set sync (was already correct).
• extproc — now diffs pctx.Headers against a clone and emits a SetHeaders for every mutation, not just Authorization.
• forwardproxy — same full-set propagation.

Guard tests were added with #760 and live in the branch: authbridge/authlib/listener/extproc/server_headerdiff_test.go and .../forwardproxy/server_headerdiff_test.go (the extproc one asserts a dg-parent=… tracestate write survives to the wire). So the outbound peer stamp does leave the sidecar, and there's no silent-flattening risk or #760-lands-first ordering to state in the PR body. Thanks for catching it while it was real.

suggestion — the export is unconditionally plaintext, and config.go strips the scheme that would ask for otherwise.

There is no TLS path here at all: insecure.NewCredentials() is the only transport credential. Meanwhile decodeConfig strips both prefixes (config.go:76-77):

cfg.OTelEndpoint = strings.TrimPrefix(cfg.OTelEndpoint, "https://")
cfg.OTelEndpoint = strings.TrimPrefix(cfg.OTelEndpoint, "http://")

So otel_endpoint: https://collector.example.com:4317 is accepted, silently reduced to host:port, and exported in cleartext to a remote host. Stripping http:// is reasonable; stripping https:// without honouring it converts an explicit request for encryption into its opposite.

The default localhost:4317 is why I am not calling this blocking. But the exposure is not limited to capture_io: lineage.principal.sub and lineage.principal.client are emitted on every inbound request span whenever a JWT validated (lines 538-543) and are not gated by capture_io. So user subject identifiers cross the network unencrypted the moment a remote endpoint is configured — with capture_io on, so do user messages, tool arguments, and LLM completions.

That also undercuts the mitigation the config field itself offers — "enable only if traces do not contain PII or the OTel backend enforces appropriate access controls" — since backend access controls are no help against a cleartext transport.

Two clean options: reject a https:// endpoint at Configure time (fail closed, consistent with the DisallowUnknownFields choice already made in this package), or honour it with real TLS credentials.

Addressed in 4f4e31c6. The export is no longer unconditionally plaintext: Config now carries an otel_tls knob, config.go parses the endpoint with url.Parse instead of stripping prefixes, and an https:// scheme auto-enables TLS (dialing with system root CAs) rather than being silently reduced to host:port. The two failure modes you called out are now closed:

• https:// + otel_tls: false is a rejected contradiction at Configure time (fails closed, matching the DisallowUnknownFields posture you noted).
• Any non-http(s) scheme (ftp://, ftps://, …) is rejected at decode rather than stripped and dialed insecure.

So a https:// endpoint now gets real encryption, and the principal.sub / principal.client facts (which, as you noted, aren't gated by capture_io) no longer cross the network in cleartext when a remote endpoint is configured. Default stays localhost:4317 plaintext for the in-pod loopback case. Covered by TestConfig_TLSFromScheme in plugin_test.go.

suggestion — the normative spec for this plugin's output is not reviewable from this repository.

The doc comment describes docs/sidecar-wire-contract.md in the lab-data-governance repo as "the law this file implements", and tracestateStampKey = "dg-parent" (line 84) names that consuming system — renamed from kglin on 2026-08-04. So the span vocabulary, the attribute set, and the parent-precedence rules are all specified somewhere a cortex reviewer cannot read, and can change without any signal here.

That matters more than usual for two reasons. First, this plugin does not merely observe: line 361 writes a vendor-specific member into the tracestate of requests forwarded to peers, so a contract change alters traffic leaving the sidecar. Second, cortex auto-syncs into productization, so "experimental plugin for one consumer" and "shipped surface" are not cleanly separable here.

Not a code problem, and the plugin is honestly scoped (facts-only, no vocabulary, build-tag excludable, inert unless configured). But it seems better as an explicit maintainer decision than an implicit one — either vendoring the relevant contract section into authbridge/docs/, or pinning the cited version somewhere that breaks loudly when the consumer moves.

Good point, and rather than just pinning the version I'd like to fix the part that actually bothers you here: the tracestate key naming a specific external consumer.

The key's function is narrow and self-contained: it's the single tracestate member the sidecar chain uses to carry its own parent link from one lineage element to the next — inbound stamps the request it forwards to its app; the app's propagate-only shim couriers the member along the request's causal chain; the peer's outbound re-stamps it so the next sidecar's inbound reads it as its parent. It carries one value (the upstream request span's id), never lands in stored data, and is independent of any particular consumer — the current name (dg-parent, formerly kglin) is just historical baggage from where the first consumer lived.

So I think it should be a neutral, producer-owned name rather than one that names data-governance — which I believe answers your concern directly (the owner on the wire becomes this plugin / authbridge, not an external system). Since it's a cross-repo wire contract, I don't want to rename it unilaterally: what would you name it? Given its function above — a producer-owned sidecar-parent-chain member — something like parent or chain-parent is where my head is, but I'd rather take your suggestion. Once we agree a name, I'll change it here and on the receiver side in the same coordinated release so the parent-join never sees a mismatch.

suggestion: insecure.NewCredentials() is hardcoded — there's no way to enable TLS even for a production otel_endpoint pointing outside the pod. Consider adding a otel_insecure: true/false (default true for the loopback default) config key, or at minimum document that TLS is currently unsupported. For in-cluster loopback-only deployments this is fine; for any cross-node or external collector endpoint it silently sends traces over plaintext.

Fixed in 4f4e31c6. There's now a real TLS path: an otel_tls config knob, and config.go parses the endpoint with url.Parse instead of stripping prefixes. An https:// endpoint auto-enables TLS (system root CAs); an explicit otel_tls: false alongside https:// is rejected at Configure time (fail-closed, matching the DisallowUnknownFields posture); any non-http(s) scheme is rejected rather than dialed insecure. Default stays localhost:4317 plaintext for the in-pod loopback case. Covered by TestConfig_TLSFromScheme.

nit: "http.method" is the deprecated OTel semconv attribute (stable since v1.21 as http.request.method). Since the plugin intentionally uses its own vocabulary as a contract (lineage.*), this is fine if intentional — but if interop with standard OTel tooling is a goal, the stable key is http.request.method. Similarly "http.status_code" (line 424) vs stable http.response.status_code. Worth a comment clarifying intent.

Intentional, and now documented inline. This producer's contract vocabulary is lineage.* plus these two well-known HTTP keys, pinned to the wire contract rather than the stable OTel names — interop with generic OTel tooling is a stated non-goal here. Added comments at plugin.go:559-561 (http.method) and 465-471 (http.status_code) clarifying the intent, per your suggestion.

suggestion: The PR body explicitly calls this out ("No producer-side payload size cap"), but there's no runtime safeguard: with capture_io: true, a large LLM completion or A2A message goes into an OTel span attribute whole. OTel SDK will silently drop attributes that exceed the exporter's max attribute size (OTLP default 4096 bytes). A truncation to e.g. 4KB with a …[truncated] suffix would make the behavior explicit and predictable rather than silently lossy at the exporter layer.

Fixed in 4f4e31c6. There's now a runtime truncate() on both input.value and output.value, bounded by a MaxPayloadBytes config key, appending a …[truncated] suffix — so oversized payloads are explicitly and predictably cut here rather than silently dropped at the exporter's attribute-size limit.

nit: The >>> OPTION-4 DELETION POINT <<< comment is helpful context for a fork/variant, but it's somewhat confusing as production inline documentation since the variant doesn't exist yet. Consider moving it to the package doc or a HACKING.md note rather than decorating live code paths with placeholder surgery instructions.

Fixed in 4f4e31c6. The placeholder-surgery marker is out of the live code path; the read-only "Option 4" variant is now described in the package doc (plugin.go:30-34) with a one-line back-reference at the relevant call site (:324) rather than an inline deletion instruction. Thanks.

suggestion: The budget <= 0 fallback does a raw byte slice (s[:max]) that can split a multi-byte UTF-8 rune when max is smaller than len(truncatedSuffix) (14 bytes). The comment says the returned string "never exceeds max bytes" but says nothing about rune-safety on this path. Consider using utf8.RuneError-safe trimming here too, or at minimum document that this edge case produces potentially invalid UTF-8 (realistically only hit with absurdly small max_payload_bytes, but the TestTruncate multi-byte case doesn't exercise budget <= 0).

Fixed in 87fdac55. The budget <= 0 fallback path now walks back to a rune boundary with utf8.RuneStart before slicing (plugin.go:622-627), the same rune-safe trim used on the normal path, so it can no longer return invalid UTF-8 even at an absurdly small max_payload_bytes.

nit: Shutdown() doesn't reset p.ready to false. After tp.Shutdown() the OTel SDK returns no-op spans, so this won't crash, but Ready() will keep returning true post-shutdown — potentially misleading to a pipeline orchestrator checking readiness before routing traffic. Consider p.ready.Store(false) as the first line of Shutdown() to make the lifecycle observable.

Fixed in db68cfbc. p.ready.Store(false) is now the first line of Shutdown (plugin.go:265), so Ready() returns false after shutdown and the lifecycle transition is observable to a pipeline orchestrator. Thanks.

nit — the version in this citation is stale, and this is the one comment in the file where that matters.

The rebase onto upstream/main bumped the pin: authbridge/authlib/go.mod now has go.opentelemetry.io/otel v1.46.0 (and sdk/trace/otlptracegrpc alongside it). The comment says v1.44.

Ordinarily that would be pure pedantry, but this paragraph deliberately grounds "a caller cannot make Insert fail" — and therefore the decision to leave the refusal branch as a guard rather than a handled case — in version-specific SDK behaviour. A claim that is pinned on purpose should cite the pin that is actually in the go.mod.

I checked the behaviour at v1.46 rather than assume the bump was inert, and it is unchanged, so nothing else here needs touching:

// trace/tracestate.go:300-311 @ v1.46.0
cTS := TraceState{}
if found == n && n < maxListMembers {   // 32 < 32 → false at the cap
	cTS.list = make([]member, n+1)
} else {
	cTS.list = make([]member, n)       // same length: room made by dropping one
}
cTS.list[0] = m
// When the number of members exceeds capacity, drop the "right-most".
copy(cTS.list[1:], ts.list[0:found])

So the eviction is real, it is right-most, and Insert still errors only out of newMember (invalid key or value) — both of ours are a constant and a span id. s/v1.44/v1.46/ is the whole fix.

nit — "countable" overstates what this counter is reachable by.

exportFailures has a route out of the process: Metrics() publishes it as lineage.export_failures on /v1/pipeline. stampRefusals has none — Metrics() returns a single-element slice — so today it is observable only from a debugger or a heap dump. The throttling half of the sentence is accurate and is exactly what I asked for; it is the second clause that no longer matches the code.

Either ending is fine by me, and I don't think this one is worth much of your time:

• add the second pipeline.Metric beside the first, which makes the word true and is ~6 lines; or
• reword to something like "…so the WARN on that branch is throttled" and drop "countable", leaving the atomic as the throttle's state and nothing more.

I'd lean to the reword given your own argument two paragraphs down in restampTracestate — if the branch guards an SDK contract change rather than a wire shape, a permanently-zero operator-facing metric is arguably worse than no metric. Raising it only because this PR has been notably careful about keeping comments and code in lockstep (2b4c0f8, c5cd817), so drift stands out more here than it would elsewhere.

nit — this test cannot pass for the wrong reason, but it can fail for one.

The design is right: 300 attempts at a nanosecond-wide window, and if the awaitIdentity ordering were wrong some plugin ends up ready, so there is no false-pass. What makes it a probabilistic failure is that the verification is a single sample after a fixed budget:

time.Sleep(200 * identityPollInterval)   // 10ms
for i, p := range plugins {
	if p.Ready() { t.Fatalf(...) }
}

The poller's transient is ready.Store(true) → ctx.Err() → ready.Store(false). A goroutine descheduled inside those three statements reports Ready() == true while still being perfectly correct — it just hasn't run its second store yet. Ten milliseconds is generous against 50µs ticks, but shared CI runners do stall goroutines for tens of ms, and there is no synchronisation tying the sleep to "all 300 pollers have finished", only to the poll interval.

Cheapest fix that keeps the shape: assert stability instead of sampling once — require !Ready() to hold across a short bounded re-check (or over a couple of consecutive passes), so a mid-window observation retries rather than fails. waitReady right above already has the polling-with-deadline idiom to mirror.

Secondary, and take it or leave it: the loop stands up 300 real TracerProviders, each with a batch-processor goroutine and a gRPC client to localhost:4317, and calls Shutdown on every one. It's harmless today because no spans are ever recorded so nothing dials, but it makes this the most expensive test in the file by a wide margin for a property that is about two atomics and a context. It also can't be avoided without an exporter seam in Init, so I mention it as a note on where a seam would pay off, not as something to fix here.